refactor: split browser and auth-proxy ingresses. by pantierra · Pull Request #586 · developmentseed/eoapi-k8s

pantierra · 2026-07-23T23:42:49Z

Closes #568.

claude · 2026-07-23T23:43:09Z

Claude finished @pantierra's task in 3m 8s —— View job

Claude's Review

Fetch and review full diff (origin/main...HEAD)
Review templates/_helpers/networking.tpl changes
Review new ingress templates (auth-proxy, browser) and modified ingress.yaml/traefik-middleware.yaml
Review test files for correctness
Apply ponytail-review for over-engineering findings
Post final review

✅ No blocking issues — safe to merge.

Both previously-requested changes are addressed:

The notContains assertions across ingress_test.yaml, stac-auth-proxy-ingress_test.yaml, and stac_browser_tests.yaml now all carry any: true, so they actually fail when the path is present (verified none were missed).
eoapi.ingressCommonAnnotations (charts/eoapi/templates/_helpers/networking.tpl:83) now merges a user-supplied traefik.ingress.kubernetes.io/router.middlewares with the chart-owned value (user,owned) instead of silently overwriting it, and this is covered by the updated test in ingress_test.yaml:446 ("chart middlewares merge with user router.middlewares annotation").

Traced the refactor end-to-end (eoapi.ingressFlags, eoapi.ingressManifest, the three ingress templates, and the Traefik middleware) against the test suite:

Browser is fully split out of eoapi.enabledIngressServices onto its own rewrite-free ingress; no dangling references to the removed eoapi.hasEnabledService/eoapi.traefikStripPrefixes helpers remain.
NGINX auth-proxy passthrough (auth-proxy-ingress.yaml, gated by passthroughIngress) vs. Traefik keeping auth-proxy on the main ingress (and correctly omitting /stac from strip-prefix, since stripPath is false when useAuthProxy is true) both check out against their respective tests.
The omit mechanism in eoapi.ingressManifest correctly strips nginx rewrite-target/use-regex annotations from the browser/auth-proxy variants even if a user supplies them globally via ingress.annotations, matching the "strips nginx rewrite annotations from the browser ingress" test.
No nil-map risk from the new $entry.config | default (index $root.Values $entry.key) pattern for mockOidcServer — testing.mockOidcServer exists in values.yaml and all sub-key access goes through | default dict.

Note: I wasn't able to execute helm unittest/ct lint in this sandbox (no shell execution permission beyond read-only git), so this is a static review only — the described test updates look internally consistent with the template logic, but a green CI run should still be the final confirmation.

Simplify (ponytail)

charts/eoapi/templates/_helpers/networking.tpl:120 (eoapi.ingressCommonAnnotations) — the omit param is generic (arbitrary key list) but only ever called with one hardcoded list (nginxRewriteKeys) from eoapi.ingressManifest; could inline two unset calls in ingressManifest after the include instead of threading a generic list through both templates. Minor, not worth blocking on.

💰 Estimated review cost: $0.73 · 3m08s · 32 turns

lhoupert · 2026-07-29T14:33:30Z

+      - notContains:
+          path: spec.rules[0].http.paths
+          content:
+            path: /stac
+      - equal:
+          path: metadata.annotations["nginx.ingress.kubernetes.io/rewrite-target"]
+          value: /$2


It seems that these notContains assertions can never fail without any: true
The assertion seems to pass even when the path is present, and fails correctly once any: true is added.

lhoupert · 2026-07-29T14:35:13Z

+      - notContains:
          path: spec.rules[0].http.paths
          content:
-            pathType: Prefix
            path: /stac
-            backend:
-              service:
-                name: RELEASE-NAME-stac-auth-proxy
-                port:
-                  number: 8080
-      - contains:
+      - notContains:
          path: spec.rules[0].http.paths
          content:
-            pathType: ImplementationSpecific
-            path: /browser(/|$)(.*)
-            backend:
-              service:
-                name: RELEASE-NAME-browser
-                port:
-                  number: 8080
+            path: /browser


SAme issue as above

lhoupert · 2026-07-29T14:39:03Z

+      - notContains:
          path: spec.rules[0].http.paths
          content:
-            pathType: Prefix
            path: /stac
-            backend:
-              service:
-                name: RELEASE-NAME-stac-auth-proxy
-                port:
-                  number: 8080
-      - contains:
+      - notContains:
          path: spec.rules[0].http.paths
          content:
-            pathType: ImplementationSpecific
-            path: /browser(/|$)(.*)
-            backend:
-              service:
-                name: RELEASE-NAME-browser
-                port:
-                  number: 8080
+            path: /browser


Suggested change

- notContains:

path: spec.rules[0].http.paths

content:

pathType: Prefix

path: /stac

backend:

service:

name: RELEASE-NAME-stac-auth-proxy

port:

number: 8080

- contains:

- notContains:

path: spec.rules[0].http.paths

content:

pathType: ImplementationSpecific

path: /browser(/|$)(.*)

backend:

service:

name: RELEASE-NAME-browser

port:

number: 8080

path: /browser

- notContains:

path: spec.rules[0].http.paths

content:

path: /stac

any: true

- notContains:

path: spec.rules[0].http.paths

content:

path: /browser

any: true

- notContains:

path: spec.rules[0].http.paths

content:

path: /browser(/|$)(.*)

any: true

lhoupert · 2026-07-29T14:52:09Z

+      - notContains:
+          path: spec.rules[0].http.paths
+          content:
+            path: "/browser"


Sane issue than https://github.com/developmentseed/eoapi-k8s/pull/586/changes#r3675257745

Suggested change

- notContains:

path: spec.rules[0].http.paths

content:

path: "/browser"

- notContains:

path: spec.rules[0].http.paths

content:

path: "/browser"

any: true

- notContains:

path: spec.rules[0].http.paths

content:

path: "/browser(/|$)(.*)"

any: true

lhoupert · 2026-07-29T14:54:13Z

+      - notContains:
+          path: spec.rules[0].http.paths
+          content:
+            path: /stac


Suggested change

- notContains:

path: spec.rules[0].http.paths

content:

path: /stac

- notContains:

path: spec.rules[0].http.paths

content:

path: /stac

any: true

- notContains:

path: spec.rules[0].http.paths

content:

path: /stac(/|$)(.*)

any: true

lhoupert · 2026-07-30T14:04:21Z

+{{- $_ := set $annotations "traefik.ingress.kubernetes.io/router.entrypoints" ($root.Values.ingress.entrypoints | toString) -}}
+{{- end -}}
+{{- $annotations = mergeOverwrite $annotations ($root.Values.ingress.annotations | default dict) -}}
+{{- $annotations = mergeOverwrite $annotations $owned -}}


While testing this branch with Claude it noticed that a user supplied traefik.ingress.kubernetes.io/router.middlewares in ingress.annotations can get silently dropped

To reproduce:

helm template eoapi charts/eoapi \ --set ingress.className=traefik \ --set browser.enabled=true --set raster.enabled=true \ --set 'ingress.annotations.traefik\.ingress\.kubernetes\.io/router\.middlewares=default-forward-auth@kubernetescrd'

Ingress router.middlewares value rendered

eoapi-browser-ingress default-eoapi-browser-redirect-middleware@kubernetescrd

eoapi-ingress (main) default-eoapi-strip-prefix-middleware@kubernetescrd

default-forward-auth@kubernetescrd, the value set isn't in either one. It's not appended or merged, just replaced, with no warning 😬

Would it make sense to append instead of overwrite? e.g. in eoapi.ingressCommonAnnotations, just before the final merge:

{{- $mwKey := "traefik.ingress.kubernetes.io/router.middlewares" -}} {{- if and (hasKey $annotations $mwKey) (hasKey $owned $mwKey) -}} {{- $_ := set $owned $mwKey (printf "%s,%s" (get $annotations $mwKey) (get $owned $mwKey)) -}} {{- end -}}

With that change, the same command renders:

Ingress router.middlewares value rendered

eoapi-browser-ingress default-forward-auth@kubernetescrd,default-eoapi-browser-redirect-middleware@kubernetescrd

eoapi-ingress (main) default-forward-auth@kubernetescrd,defauleware@kubernetescrd

what do you think?

pantierra · 2026-08-25T11:20:59Z

Thanks for the review, @lhoupert! I addressed the mentioned issues and cleaned up a bit more. I'd appreciate another round of review.

pantierra force-pushed the fix/unified-ingress-routing branch 2 times, most recently from b2068f8 to 652a443 Compare July 24, 2026 00:07

This was referenced Jul 24, 2026

refactor: extract unified ingress helpers. #546

Merged

Enabling stac-auth-proxy on Traefik breaks routing twice: dangling strip-prefix middleware (whole host 404s), and probes miss ROOT_PATH (pod never Ready) #568

Open

pantierra force-pushed the fix/unified-ingress-routing branch 3 times, most recently from 0c5165d to c4bb6ea Compare July 24, 2026 00:34

pantierra requested a review from lhoupert July 24, 2026 00:41

pantierra marked this pull request as ready for review July 24, 2026 00:41

pantierra force-pushed the fix/unified-ingress-routing branch 2 times, most recently from 364dceb to 81e90d4 Compare July 24, 2026 00:46

lhoupert reviewed Jul 29, 2026

View reviewed changes

lhoupert reviewed Jul 30, 2026

View reviewed changes

lhoupert requested changes Jul 30, 2026

View reviewed changes

pantierra mentioned this pull request Aug 24, 2026

Deploying using nginx ingress results in 404 for /browser #593

Open

feat: split browser and auth-proxy ingresses.

8470238

pantierra force-pushed the fix/unified-ingress-routing branch 2 times, most recently from 52bb6b6 to a7e5d7d Compare August 25, 2026 11:11

pantierra requested a review from lhoupert August 25, 2026 11:18

pantierra force-pushed the fix/unified-ingress-routing branch from a7e5d7d to 7fc2658 Compare August 25, 2026 11:20

merge user router.middlewares annotation instead of overwriting it

c07ea81

pantierra force-pushed the fix/unified-ingress-routing branch from 7fc2658 to c07ea81 Compare August 25, 2026 11:36

lhoupert mentioned this pull request Aug 25, 2026

chore(deps): upgraded stac-browser to version 5.0.0. #598

Open

pantierra force-pushed the fix/unified-ingress-routing branch from b55d3fa to c07ea81 Compare August 25, 2026 13:10

Navigation Menu

Uh oh!

refactor: split browser and auth-proxy ingresses. - #586

refactor: split browser and auth-proxy ingresses.#586
pantierra wants to merge 2 commits into
mainfrom
fix/unified-ingress-routing

pantierra commented Jul 23, 2026 •

edited

Loading

Uh oh!

claude Bot commented Jul 23, 2026 •

edited by github-actions Bot

Loading

Uh oh!

lhoupert Jul 29, 2026

Uh oh!

pantierra Aug 25, 2026

Uh oh!

lhoupert Jul 29, 2026 •

edited

Loading

Uh oh!

lhoupert Jul 29, 2026 •

edited

Loading

Uh oh!

lhoupert Jul 29, 2026 •

edited

Loading

Uh oh!

lhoupert Jul 29, 2026

Uh oh!

lhoupert Jul 30, 2026

Uh oh!

pantierra Aug 25, 2026

Uh oh!

pantierra commented Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Ingress	router.middlewares value rendered
eoapi-browser-ingress	default-eoapi-browser-redirect-middleware@kubernetescrd
eoapi-ingress (main)	default-eoapi-strip-prefix-middleware@kubernetescrd

Ingress	router.middlewares value rendered
eoapi-browser-ingress	default-forward-auth@kubernetescrd,default-eoapi-browser-redirect-middleware@kubernetescrd
eoapi-ingress (main)	default-forward-auth@kubernetescrd,defauleware@kubernetescrd

Uh oh!

Conversation

pantierra commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Jul 23, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude's Review

Uh oh!

lhoupert Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

pantierra Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

lhoupert Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhoupert Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhoupert Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhoupert Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

lhoupert Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

pantierra Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

pantierra commented Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pantierra commented Jul 23, 2026 •

edited

Loading

claude Bot commented Jul 23, 2026 •

edited by github-actions Bot

Loading

lhoupert Jul 29, 2026 •

edited

Loading

lhoupert Jul 29, 2026 •

edited

Loading

lhoupert Jul 29, 2026 •

edited

Loading